feat(@mcp/hono): mcpHonoHandler mount helper (handleHttp-backed)#2074
feat(@mcp/hono): mcpHonoHandler mount helper (handleHttp-backed)#2074felixweinberger wants to merge 1 commit into
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
1 similar comment
|
@claude review |
| * app.all('/mcp', mcpHonoHandler(mcp, { session: new SessionCompat() })); | ||
| * ``` | ||
| */ | ||
| export function mcpHonoHandler(mcp: Dispatchable, options?: HandleHttpOptions): Handler<{ Variables: McpHonoVariables }> { |
There was a problem hiding this comment.
🟡 This PR adds two new public exports (mcpHonoHandler and McpHonoVariables, surfaced via export * from './hono.js' in index.ts), but the PR body says "Public API unchanged" / "Documentation update" only, no .changeset is included (changeset-bot flagged this — should be a minor for @modelcontextprotocol/hono), and packages/middleware/hono/README.md still lists only createMcpHonoApp / hostHeaderValidation / localhostHostValidation and demonstrates the legacy WebStandardStreamableHTTPServerTransport + server.connect() pattern. Please add a changeset, correct the PR description, and update the README's Exports + Usage sections — the JSDoc snippet on mcpHonoHandler (hono.ts:23-30) is ready to copy in.
Extended reasoning...
What the issue is. packages/middleware/hono/src/index.ts does export * from './hono.js', so the newly added export function mcpHonoHandler(...) and export type McpHonoVariables become part of @modelcontextprotocol/hono's public API surface. CLAUDE.md § Public API Exports is explicit: "Adding a symbol to a package index.ts makes it public API — do so intentionally." The PR title itself agrees — it's feat(@mcp/hono): mcpHonoHandler mount helper. Yet three release-hygiene artifacts were not updated to match.
Step-by-step proof.
src/index.ts:1→export * from './hono.js're-exports everything fromhono.ts.- The diff adds
export type McpHonoVariables(hono.ts:13) andexport function mcpHonoHandler(hono.ts:32) — both are therefore public. git diff HEAD~1..HEAD --name-onlyshows onlyhono.tsandhono.test.ts— no.changeset/*.mdand noREADME.mdchange. changeset-bot in the PR timeline confirms "No Changeset found" for commit 20b17c0.- The PR description states "Public API unchanged" and ticks only "[x] Documentation update", contradicting both the diff and the PR title.
packages/middleware/hono/README.mdlines 19–23 list onlycreateMcpHonoApp,hostHeaderValidation,localhostHostValidationunder Exports —mcpHonoHandlerandMcpHonoVariablesare absent.- README lines 29–39 still show
WebStandardStreamableHTTPServerTransport+server.connect(transport)+ manualtransport.handleRequest(c.req.raw, { parsedBody: c.get('parsedBody') })— exactly the patternmcpHonoHandleris meant to supersede.
Why nothing currently catches it. The repo's review checklist says "New feature: verify prose documentation is added (not just JSDoc)" and the Documentation & Changesets recurring catch says to flag prose that the diff doesn't back. JSDoc was added (hono.ts:15–31), but neither the package README nor a changeset was, and the PR body actively claims no API change. Tests and typecheck pass because none of this is enforced by CI beyond changeset-bot's non-blocking warning.
Impact. Without a changeset, the next @modelcontextprotocol/hono release won't get a version bump or changelog entry for the new mount helper, so consumers reading the changelog/README won't discover mcpHonoHandler and will keep wiring the legacy transport manually. The misleading "Public API unchanged" claim also makes the PR harder to audit later. This is process/release hygiene rather than a runtime defect, hence nit severity. (Noted: this is part of the stacked v2-stateless series — "Depends on R4. Hold until R/S/F have soaked" — so the author may intend to batch changesets, but the PR-body claim and README should still be corrected here.)
How to fix.
- Add a changeset, e.g.
.changeset/lucky-mangos-rescue.md:--- "@modelcontextprotocol/hono": minor --- Add `mcpHonoHandler(mcp, options?)` mount helper and `McpHonoVariables` type, backed by `handleHttp`.
- Update the PR description: tick New feature, drop "Public API unchanged".
- Update
packages/middleware/hono/README.md: addmcpHonoHandler(mcp, options?)andMcpHonoVariablesto the Exports list, and replace the Usage example with the snippet already in the JSDoc:import { McpServer, SessionCompat } from '@modelcontextprotocol/server'; import { createMcpHonoApp, mcpHonoHandler } from '@modelcontextprotocol/hono'; const mcp = new McpServer({ name: 's', version: '1.0.0' }); const app = createMcpHonoApp(); app.all('/mcp', mcpHonoHandler(mcp, { session: new SessionCompat() }));
| * ```ts | ||
| * import { McpServer, SessionCompat } from '@modelcontextprotocol/server'; | ||
| * import { createMcpHonoApp, mcpHonoHandler } from '@modelcontextprotocol/hono'; | ||
| * | ||
| * const mcp = new McpServer({ name: 's', version: '1.0.0' }); | ||
| * const app = createMcpHonoApp(); | ||
| * app.all('/mcp', mcpHonoHandler(mcp, { session: new SessionCompat() })); | ||
| * ``` |
There was a problem hiding this comment.
🟡 nit: per CLAUDE.md § "JSDoc @example Code Snippets", this code block should be an @example with a ts source="./hono.examples.ts#…" fence pulling from a type-checked companion file (as express.ts, fastify.ts, and node/streamableHttp.ts all do). Right now there's no packages/middleware/hono/src/hono.examples.ts, so this inline snippet isn't type-checked and will silently rot if mcpHonoHandler / SessionCompat signatures change. Consider adding a hono.examples.ts with e.g. a #region mcpHonoHandler_basicUsage and referencing it via source=.
Extended reasoning...
What the issue is. CLAUDE.md (§ JSDoc @example Code Snippets, lines ~44–48) states that JSDoc example code should pull type-checked code from companion .examples.ts files using fences of the form ts source="./file.examples.ts#regionName", kept in sync via pnpm sync:snippets. The new mcpHonoHandler JSDoc instead embeds a plain ts fence with an inline, hand-written snippet (lines 23–30), with no @example tag and no source= attribute.
Why the convention exists / why it matters here. The point of the source= + .examples.ts mechanism is that the snippet is real, compiled TypeScript — if mcpHonoHandler's signature changes, or SessionCompat is renamed, or createMcpHonoApp gains a required option, tsc fails on hono.examples.ts and the breakage is caught in CI. An inline snippet has none of that protection: it can drift from the actual API and nothing in the build will notice. This is the first JSDoc code block in hono.ts, so it also sets the precedent for the package.
Sibling-package precedent. Every other middleware adapter already follows the convention:
packages/middleware/express/src/express.ts→@example+source="./express.examples.ts#…", backed byexpress.examples.tspackages/middleware/fastify/src/fastify.ts→@example+source="./fastify.examples.ts#…", backed byfastify.examples.tspackages/middleware/node/src/streamableHttp.ts→source="./streamableHttp.examples.ts#…", backed bystreamableHttp.examples.ts
@modelcontextprotocol/hono is the only middleware package with no *.examples.ts file at all (verified via glob), so this PR is a natural place to add one rather than diverge further.
Step-by-step illustration of the rot risk.
- Today the snippet reads
mcpHonoHandler(mcp, { session: new SessionCompat() }). - Suppose a later PR renames
SessionCompat→SessionManager, or changes the option key fromsessiontosessions. pnpm build/pnpm typecheckrun cleanly because nothing compiles this string — it's just a comment.pnpm sync:snippetsalso does nothing, because there's nosource=attribute pointing at a region to re-sync.- The published JSDoc / generated docs now show code that doesn't compile, and the only way to catch it is a human re-reading the comment. With a
hono.examples.tsregion, step 3 would instead fail the build.
Suggested fix. Add packages/middleware/hono/src/hono.examples.ts containing the snippet inside // #region mcpHonoHandler_basicUsage / // #endregion, then change the JSDoc to:
* @example
* ```ts source="./hono.examples.ts#mcpHonoHandler_basicUsage"
* import { McpServer, SessionCompat } from '@modelcontextprotocol/server';
* import { createMcpHonoApp, mcpHonoHandler } from '@modelcontextprotocol/hono';
*
* const mcp = new McpServer({ name: 's', version: '1.0.0' });
* const app = createMcpHonoApp();
* app.all('/mcp', mcpHonoHandler(mcp, { session: new SessionCompat() }));
* ```and run pnpm sync:snippets.
Severity. This is a docs/style convention nit, not a correctness defect — the handler itself is fine. Filing as nit. (Minor caveat: a couple of pre-existing bare ts fences exist elsewhere in the repo, e.g. node/streamableHttp.ts:34 and express/auth/metadataRouter.ts:108, so enforcement isn't absolute — but the dominant pattern across the middleware adapters is clearly source= + .examples.ts, and CLAUDE.md documents it as the expected approach.)
3eebc7f to
2358111
Compare
20b17c0 to
8d8cf7e
Compare
|
@claude review |
2358111 to
8493aac
Compare
8d8cf7e to
22dfcc9
Compare
|
@claude review |
8493aac to
716039a
Compare
22dfcc9 to
bdf79c7
Compare
|
@claude review |
@modelcontextprotocol/honoadapter switches internals tohandleHttp(server). Public API unchanged.Motivation and Context
Same as M1, for hono.
How Has This Been Tested?
Existing
@modelcontextprotocol/honotests pass unchanged.Breaking Changes
None.
Types of changes
Checklist
Additional context
Depends on R4. Hold until R/S/F have soaked. Review guide: https://gist.github.com/felixweinberger/5a48e0f14d5aced39ed6a91b61940711.